Add strict TOTP validation to block invalid authentication requests #578
Conversation
| try: | ||
| print("Enter TOTP: ", end="", flush=True) | ||
| ready, _, _ = select.select([sys.stdin], [], [], timeout_seconds) | ||
| if ready: |
There was a problem hiding this comment.
Should we wait 5mins for the user to enter TOTP?
There was a problem hiding this comment.
Hi Siva, if the user does not provide the TOTP within 5 minutes, the session should expire—meaning the connection between the driver and the server should be terminated.
vertica_python/vertica/connection.py
Outdated
| result = validate_totp_code(totp_input, totp_is_valid=None) | ||
| if not result.ok: | ||
| msg = result.message or INVALID_TOTP_MSG | ||
| print(msg) |
There was a problem hiding this comment.
is this print(msg) for debugging purpose, please remove it
There was a problem hiding this comment.
Hi Siva, I have removed the debug message .
|
MR description did not cover all the issues that addressed. Please update the MR description accordingly. |
vertica_python/vertica/connection.py
Outdated
| INVALID_TOTP_MSG = 'Invalid TOTP: Please enter a valid 6-digit numeric code.' | ||
|
|
||
| if validate_totp_code is not None: | ||
| result = validate_totp_code(self.totp, totp_is_valid=None) |
There was a problem hiding this comment.
validate_totp_code function definition is missing.
There was a problem hiding this comment.
Hi Siva , Added the definition for validate_totp_code.
| # Remove common separators inside the code | ||
| # Spaces, hyphens, underscores, dots, and common dash-like characters | ||
| separators = {' ', '\t', '\n', '\r', '\f', '\v', '-', '_', '.', | ||
| '\u2012', '\u2013', '\u2014', '\u2212', '\u00B7', '\u2027', '\u30FB'} |
There was a problem hiding this comment.
If user enters the TOTP with - or _ or ., your code is removing them without giving error message. Removing the trailing or leading white spaces is acceptable, but if user deliberately enters these special characters, should we remove them or should we warn user that invalid characters are not allowed.
| # All good | ||
| return TotpValidationResult(True, s, '') | ||
| except Exception: | ||
| # Fallback generic error |
There was a problem hiding this comment.
There is no exception handling, it is just like function return. Handle the exception when we use try catch block
|
|
||
|
|
||
| def validate_totp_code(raw_code: str, totp_is_valid=None) -> TotpValidationResult: | ||
| """Validate and normalize a user-supplied TOTP value. |
There was a problem hiding this comment.
totp_is_valid is written as it is reserved for server side checks. but validate_totp_code function itself is called to do the checks before sending to the server. Something is not right, can you please verify?
|
|
||
| # All good | ||
| return TotpValidationResult(True, s, '') | ||
| except Exception: |
There was a problem hiding this comment.
why the tests are not updated to test these new error message?
sivaalamp
left a comment
sivaalamp
left a comment
There was a problem hiding this comment.
Tests are not updated
This MR improves TOTP validation in the Vertica Python driver by enforcing strict 6-digit numeric checks.
Invalid TOTPs (alphanumeric, incorrect length, or with extra spaces) are now blocked locally and return a consistent error message:
“Invalid TOTP: Please enter a valid 6-digit numeric code.”
This prevents invalid authentication requests from being sent to the server and improves user experience.